Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Address #252 - Make soft ranking notebook self contained #301

Merged
merged 10 commits into from
Feb 21, 2023

Conversation

AWehenkel
Copy link
Contributor

  • Remove the examples folder.
  • Remove the fairness notebook.
  • Make the notebook for soft ranking self-contained.
  • Switch for an example with MNIST to make things runnable in a decent amount of time on CPU.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@review-notebook-app
Copy link

review-notebook-app bot commented Feb 15, 2023

View / edit / reply to this conversation on ReviewNB

michalk8 commented on 2023-02-15T18:14:32Z
----------------------------------------------------------------

Line #14.    import scipy.ndimage

Use from scipy import ndimagethe formatting later should be nicer because the line will be shorter.


@review-notebook-app
Copy link

review-notebook-app bot commented Feb 15, 2023

View / edit / reply to this conversation on ReviewNB

michalk8 commented on 2023-02-15T18:14:33Z
----------------------------------------------------------------

Line #15.    from flax.training import common_utils

Seems like common_utils are not used, please remove


@review-notebook-app
Copy link

review-notebook-app bot commented Feb 15, 2023

View / edit / reply to this conversation on ReviewNB

michalk8 commented on 2023-02-15T18:14:34Z
----------------------------------------------------------------

Line #20.    import ott

For consistency with other tutorials, please use from ott.tools import soft_sort


@review-notebook-app
Copy link

review-notebook-app bot commented Feb 15, 2023

View / edit / reply to this conversation on ReviewNB

michalk8 commented on 2023-02-15T18:14:35Z
----------------------------------------------------------------

Line #1.    url = "https://raw.githubusercontent.com/matplotlib/matplotlib/master/doc/_static/stinkbug.png"

Since we removed requests,let's use:

with urllib.request.urlopen(url) as resp:

image = plt.imread(io.BytesIO(resp.read()))


@review-notebook-app
Copy link

review-notebook-app bot commented Feb 15, 2023

View / edit / reply to this conversation on ReviewNB

michalk8 commented on 2023-02-15T18:14:36Z
----------------------------------------------------------------

Line #1.    def salt_and_pepper(im, amount=0.05):

Completely optional, but if you want, would be great to add type hints to all the functions.


@review-notebook-app
Copy link

review-notebook-app bot commented Feb 15, 2023

View / edit / reply to this conversation on ReviewNB

michalk8 commented on 2023-02-15T18:14:37Z
----------------------------------------------------------------

Optax with capital "O"

... neural network with Jax, as well PyTorch dataset and dataloaders.


@review-notebook-app
Copy link

review-notebook-app bot commented Feb 15, 2023

View / edit / reply to this conversation on ReviewNB

michalk8 commented on 2023-02-15T18:14:38Z
----------------------------------------------------------------

Please move this function to the cell where you define train_step


@review-notebook-app
Copy link

review-notebook-app bot commented Feb 15, 2023

View / edit / reply to this conversation on ReviewNB

michalk8 commented on 2023-02-15T18:14:39Z
----------------------------------------------------------------

Also add these functions to the cell where train_step is defined.


@review-notebook-app
Copy link

review-notebook-app bot commented Feb 15, 2023

View / edit / reply to this conversation on ReviewNB

michalk8 commented on 2023-02-15T18:14:40Z
----------------------------------------------------------------

Line #1.    @flax.struct.dataclass

For consistency, use from flax import struct

and then @struct.dataset


@review-notebook-app
Copy link

review-notebook-app bot commented Feb 15, 2023

View / edit / reply to this conversation on ReviewNB

michalk8 commented on 2023-02-15T18:14:41Z
----------------------------------------------------------------

Move this below the cell below.


@review-notebook-app
Copy link

review-notebook-app bot commented Feb 15, 2023

View / edit / reply to this conversation on ReviewNB

michalk8 commented on 2023-02-15T18:14:42Z
----------------------------------------------------------------

Use pass instead of return


@review-notebook-app
Copy link

review-notebook-app bot commented Feb 15, 2023

View / edit / reply to this conversation on ReviewNB

michalk8 commented on 2023-02-15T18:14:43Z
----------------------------------------------------------------

Line #60.            log(results, step + 1, summary, train=True)

I'd either log every n-th iteration (add a new config value) or use tqdm , whichever you prefer.


@review-notebook-app
Copy link

review-notebook-app bot commented Feb 15, 2023

View / edit / reply to this conversation on ReviewNB

michalk8 commented on 2023-02-15T18:14:44Z
----------------------------------------------------------------

Instead of "loss", would use "soft error loss"


@review-notebook-app
Copy link

review-notebook-app bot commented Feb 15, 2023

View / edit / reply to this conversation on ReviewNB

michalk8 commented on 2023-02-15T18:14:45Z
----------------------------------------------------------------

I'd add legend for all 3 axes.

Would it be possible to train it on GPU for a bit longer and see if CE increases, as in the original notebook? Would be nice to see if we can replicate the trend in the original notebook.


AWehenkel and others added 5 commits February 16, 2023 16:24
# Conflicts:
#	examples/fairness/losses.py
#	examples/fairness/main.py
#	examples/fairness/train.py
#	examples/soft_error/data.py
#	examples/soft_error/losses.py
#	examples/soft_error/main.py
#	examples/soft_error/train.py
@michalk8
Copy link
Collaborator

Thanks @AWehenkel !

@michalk8 michalk8 merged commit 5c2f23e into ott-jax:main Feb 21, 2023
michalk8 pushed a commit that referenced this pull request Jun 27, 2024
* Test commit on personal fork.

* Test commit on personal fork.

* Update the soft sort notebook to make it self-contained.
Remove the `examples` folder.

* pre-commit the soft sort notebook

* runnable on GPUs + Michaal comments on PR

* Clean the NB.

* Clean the NB.

---------

Co-authored-by: antoinewehenkel <awehenkel@apple.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants